fix: Add exports for module compatibility #105
Conversation
| "main": "./index.js", | ||
| "types": "./index.d.ts", |
There was a problem hiding this comment.
These fields will be completely ignored once "exports" exists so they don't actually do anything.
There was a problem hiding this comment.
These are used by consumer projects with "moduleResolution": "node". We can keep both for better compatibility.
There was a problem hiding this comment.
The author of https://cmdcolin.github.io/posts/2025-01-12-pureesm says
I specify
mainandexports. Exports is sufficient for some but requires consumers use moduleResolution with nodenext which is rare
So there might be value in keeping both
There was a problem hiding this comment.
Support for the exports field is part of node16, not nodenext, so the blog is inaccurate about how rare it is. And the blog's example uses "main": "./dist/index.js", which actually does do something, unlike "main": "./index.js", which is the same as not specifying the field at all.
There's normally some value keeping "main" around (if it already existed) for most kinds of packages, but in the case of cosmjs-types, index.js is actually completely empty. index.ts re-exports 2 TypeScript types but nothing at runtime.
So only the "types" field might do something here - and only for projects that import type { DeepPartial, Exact } from "cosmjs-types" (as opposed to "cosmjs-types/helpers"), and that also still use node/node10. But it's "./index.d.ts", so to TypeScript that's the same as not specifying anything.
The fields didn't exist before this PR; if these projects worked before, they'll work without adding these 2 fields, since for them nothing will have changed. An import from a directory in Node automatically looks for /index.js, and TypeScript does the same thing with /index.d.ts.
| "import": "./index.js", | ||
| "require": "./index.js" |
There was a problem hiding this comment.
You can just put one line "default" here for now instead.
But this all conflicts with the PR for adding ESM support #104
There was a problem hiding this comment.
That's true. I will resolve if any PR for ESM merged first.
For dual format support, we would need "import" and "require" both eventually.
There was a problem hiding this comment.
Unclear if dual-format is even in the future of this package: it's very hard to maintain dual-packaging with just the tsc transpiler. There might not ever be a need for more than "default".
There was a problem hiding this comment.
Diff looks good as far as I can tell. But I have a high level question: As long as cosmjs-types is a CJS-only library (which you might disagree with but this is where we are), is there an advantage of making this change for users?
EDIT: this is doing the renaming from extensionless to .js extensions, right?
| "./*": { | ||
| "types": "./*.d.ts", | ||
| "import": "./*.js", | ||
| "require": "./*.js" |
There was a problem hiding this comment.
Before this PR, both extensionless and .js imports worked in CJS projects, and only .js worked in ESM projects. But this change will break any projects who happen to have written an import like
import { TxRaw } from "cosmjs-types/cosmos/tx/v1beta1/tx.js"Because it tries to resolve to a .js.js file that doesn't exist.
Error [ERR_MODULE_NOT_FOUND]: Cannot find module 'node_modules/cosmjs-types/cosmos/tx/v1beta1/tx.js.js'
So you need a separate entry for ./*.js for full compatibility, like in the official examples.
3 participants
There are some open PRs for ESM support but apart from that it needs proper exports to be resolved by modern bundlers.
Once it supports dual format (esm, cjs), it just need to update like:
resolves #93